-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update OIDC client and token propagation to support a jwt-bearer token grant #29130
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, added a small suggestion about the enum constant
* 'urn:ietf:params:oauth:grant-type:jwt-bearer' grant requiring an OIDC client authentication as well as | ||
* at least an 'assertion' parameter which must be passed to OidcClient at the token request time. | ||
*/ | ||
JWTBEARER("urn:ietf:params:oauth:grant-type:jwt-bearer"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JWTBEARER("urn:ietf:params:oauth:grant-type:jwt-bearer"), | |
JWT_BEARER("urn:ietf:params:oauth:grant-type:jwt-bearer"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @gastaldi One thing I've been following with these grant names is introducing the shortcuts containing -
, _
, for example, client_credentials
-> client
, urn:ietf:params:oauth:grant-type:token-exchange
-> exchange
, etc. One thing it avoids is hard to catch issues with the extra parameters which I vaguely recall we had when client
was named as client_credentials
, where the user typed extra-params.client-credentials.a=b
.
I've been thinking though earlier to have even a shorter shortcut, jwt
-> urn:ietf:params:oauth:grant-type:jwt-bearer
- perhaps we can try this one ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for keeping pushing, just noticed it :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, jwt
is much simpler, thanks for suggesting a grant name change in the first place as well
...main/java/io/quarkus/oidc/token/propagation/reactive/OidcTokenPropagationReactiveConfig.java
Outdated
Show resolved
Hide resolved
23b67b1
to
1fb0ecb
Compare
1fb0ecb
to
5280637
Compare
👍🏼 |
Fixes #18649.
This PR looks quite massive but in fact the changes are trivial, the bulk of the changes are related to adding tests to both
extensions/oidc-token-propagation/deployment
andextensions/oidc-token-propagation-reactive/deployment
. Here is a summary:OidcClientConfig
has ajwtbearer
grant type added which is a shortcut forurn:ietf:params:oauth:grant-type:jwt-bearer
AccessTokenRequestFilter
inoidc-token-propagation
can already useOidcClient
to exchange the token using the token exchange grant -OidcClient
manages it all,AccessTokenRequestFilter
only supplies the current token toOidcClient
using a property set by default tosubject_token
which is relevant in the context of the exchange token grant. The mechanics are exactly the same ifjwtbearer
has to be used to exchange the token butassertion
, instead ofsubject_token
, has to be used to supply the current token toOidcClient
- soAccessTokenRequestFilter
has been updated to deduce this property name based on the configured grantjwtbearer
so we need use Wiremock to test, I updated the testoidc-server
to have ajwtbearer
stubextensions/oidc-token-propagation/deployment
: Test client acquires a token foralice
and posts it as a bearer token toFrontendResource
which delegates further toProtectedResource
, at this pointAccessTokenRequestFilter
requests a token exchange, Wiremock stub is setup to exchange it for a token issued tobob
- andProtectedResource
return the user name from the new token, test confirms that the original token user name wasalice
but now it isbob
oidc-token-propagation-reactive
to support the token exchange exactly the same way it can be done foroidc-token-propagation
,AccessTokenRequestReactiveFilter
will suspend/resume the request thread if it needs to exchange the token; test is identical as well@gastaldi @geoand Please have a look when you get a chance, as I said, the actual main source changes are fairly simple, the complexity is within the tests only. It is RFE.
I'll also follow up with another PR to let uses register the reactive token propagation filter with the annotation shortcut, similarly to how Georgios did it for the reactive OidcClientFilter